added geotile grid support #1880

krewetka · 2020-12-01T10:15:30Z

In ElasticSearch 7 and above there is new aggregation type which is not supported here yet -> GeoTile Grid

I have added support of it. It is very similar to existing GeoHash Grid except it has more detailed precision levels.

I also renamed one test in other class which had incorrect name.

ruflin

Hi, this looks good. Could you add a changelog?

There is an other issue we have at the moment with travis and CI is not run. I would like to wait with getting this PR in until we have CI running again if that is ok for you?

krewetka · 2020-12-02T13:25:27Z

Hi, this looks good. Could you add a changelog?

sure I will add it there :)

I would like to wait with getting this PR in until we have CI running again if that is ok for you?

if this will be a week or so sure, no problem. I have switched my project to use fork right now

ruflin · 2020-12-03T09:31:37Z

@krewetka As soon as #1885 is merged, could you rebase your PR?

deguif · 2020-12-03T09:45:12Z

It's merged

deguif · 2020-12-03T10:29:42Z

src/Aggregation/GeotileGrid.php

+ *
+ * @see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-geotilegrid-aggregation.html
+ */
+class GeotileGrid extends AbstractAggregation


We should probably start marking these kind of class as final as @thePanz recently commented.
And as it's a new class also suffix its name with Aggregation. This requires #1887 to be merged before.

ok, no problem

ping @ruflin for his confirmation.

@ruflin is this ok for you? prefer confirming before doing this changes

@deguif can you point me to the final comment? I'm not sure I fully follow why we should make it final? Should we really prevent others from extending these classes?

Let's not mark the class as final for now as it would need discussing this subject more in details.
Will the name GeotileGridAggregation be OK for you?

@deguif Currently we always skip the Aggregation part as it is already in the name of the directory. So to be consistent it should be GoetileGrid?

Yes but as stated in #1824 I would be in favor to deprecate all aggregations classes and suffix them with Aggregation to avoid them colliding with reserved keywords and unifying all classes names.

Here's the class is new so it could directly be named accordingly and avoid having to deprecate it later.
Would that sound good for you?

@ruflin @deguif please align about it. I really would like it to be merged soon - one way or another ;)

I have added this Aggregation at the end per request here. I have also changed code in my project to support this new name already.

Btw, I personally like adding Aggregation to the name. Easlier my projects code looked like this

use Elastica\Aggregation\GeotileGrid as GeotileGridAggregation;

for readibility reason later in code. Otherwise it is much less visible in code where it is used that this is aggregation.

src/Aggregation/GeotileGrid.php

deguif · 2020-12-04T14:23:54Z

Thanks @krewetka for your PR.

I see on the elasticsearch documentation that there is also an optional bounds parameter, it can probably be added without too much work. WDYT?

For details: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-geotilegrid-aggregation.html#_requests_with_additional_bounding_box_filtering_2

krewetka · 2020-12-07T07:42:59Z

I see on the elasticsearch documentation that there is also an optional bounds parameter, it can probably be added without too much work. WDYT?

Yes, it could. But it also exists in geohash grid and is not there in Ruflin elastica so maybe better as separate PR adding it to both? 🤔

ruflin · 2020-12-07T07:54:51Z

@krewetka Getting an additional PR later on SGTM.

krewetka · 2020-12-07T10:14:44Z

@krewetka Getting an additional PR later on SGTM.

ok :) so this one should be ready IMO to be merged @ruflin , @deguif

krewetka · 2020-12-07T12:53:18Z

Thanks @krewetka for your PR.

I see on the elasticsearch documentation that there is also an optional bounds parameter, it can probably be added without too much work. WDYT?

For details: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-geotilegrid-aggregation.html#_requests_with_additional_bounding_box_filtering_2

@deguif after checking this parameter and info that is supports alll formats from https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-geo-bounding-box-query.html#query-dsl-geo-bounding-box-query-accepted-formats I am not sure I am able to handle this one propely.

I have checked https://github.com/ruflin/Elastica/blob/a2f5b0d7171691da06a4c31b6829b9c1dafdfbf5/src/Query/GeoBoundingBox.php and even for original one there is just one format supported now in ruflin/Eastica

I can try to add the same one type for both Geohash and Geotile but to be honest I don't have time for more, not the knowledge how to do it properly :)

deguif · 2020-12-07T14:07:37Z

@krewetka no problem for that, that's not a blocker for me.
Thank you for this nice contribution and as @ruflin already commented we can add this parameter later ;)

ruflin

LGTM. Did a commit to resolve the changelog conflict. @deguif If ok for you, please merge directly.

krewetka · 2020-12-08T14:17:21Z

Thanks for merging :)

ruflin reviewed Dec 2, 2020

View reviewed changes

deguif reviewed Dec 3, 2020

View reviewed changes

src/Aggregation/GeotileGrid.php Outdated Show resolved Hide resolved

krewetka added 2 commits December 4, 2020 10:15

added geotile grid support

dca0ca9

added geotile grid support to CHANGELOG.md

faa1486

krewetka force-pushed the geotile-feature branch 2 times, most recently from 62a42c0 to c05c160 Compare December 4, 2020 13:06

deguif mentioned this pull request Dec 7, 2020

Deprecate unsuffixed processor classes #1893

Merged

class renamed + use of new trait

146bb1b

krewetka force-pushed the geotile-feature branch from c05c160 to 146bb1b Compare December 7, 2020 12:43

Merge branch 'master' into geotile-feature

ff394fb

ruflin approved these changes Dec 8, 2020

View reviewed changes

deguif approved these changes Dec 8, 2020

View reviewed changes

deguif merged commit 50c3233 into ruflin:master Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added geotile grid support #1880

added geotile grid support #1880

krewetka commented Dec 1, 2020

ruflin left a comment

krewetka commented Dec 2, 2020 •

edited

Loading

ruflin commented Dec 3, 2020

deguif commented Dec 3, 2020

deguif Dec 3, 2020 •

edited

Loading

krewetka Dec 3, 2020

deguif Dec 3, 2020

krewetka Dec 4, 2020

ruflin Dec 4, 2020

deguif Dec 4, 2020

ruflin Dec 7, 2020

deguif Dec 7, 2020

krewetka Dec 7, 2020 •

edited

Loading

deguif commented Dec 4, 2020

krewetka commented Dec 7, 2020

ruflin commented Dec 7, 2020

krewetka commented Dec 7, 2020

krewetka commented Dec 7, 2020 •

edited

Loading

deguif commented Dec 7, 2020

ruflin left a comment

krewetka commented Dec 8, 2020

added geotile grid support #1880

added geotile grid support #1880

Conversation

krewetka commented Dec 1, 2020

ruflin left a comment

Choose a reason for hiding this comment

krewetka commented Dec 2, 2020 • edited Loading

ruflin commented Dec 3, 2020

deguif commented Dec 3, 2020

deguif Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

krewetka Dec 3, 2020

Choose a reason for hiding this comment

deguif Dec 3, 2020

Choose a reason for hiding this comment

krewetka Dec 4, 2020

Choose a reason for hiding this comment

ruflin Dec 4, 2020

Choose a reason for hiding this comment

deguif Dec 4, 2020

Choose a reason for hiding this comment

ruflin Dec 7, 2020

Choose a reason for hiding this comment

deguif Dec 7, 2020

Choose a reason for hiding this comment

krewetka Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

deguif commented Dec 4, 2020

krewetka commented Dec 7, 2020

ruflin commented Dec 7, 2020

krewetka commented Dec 7, 2020

krewetka commented Dec 7, 2020 • edited Loading

deguif commented Dec 7, 2020

ruflin left a comment

Choose a reason for hiding this comment

krewetka commented Dec 8, 2020

krewetka commented Dec 2, 2020 •

edited

Loading

deguif Dec 3, 2020 •

edited

Loading

krewetka Dec 7, 2020 •

edited

Loading

krewetka commented Dec 7, 2020 •

edited

Loading